Skip to content

Remove non-functional VideoPlayer showControlsWhenPaused prop - #1388

Merged
danielguillan merged 7 commits into
mainfrom
danielguillan/fix-video-player
Jul 16, 2026
Merged

Remove non-functional VideoPlayer showControlsWhenPaused prop#1388
danielguillan merged 7 commits into
mainfrom
danielguillan/fix-video-player

Conversation

@danielguillan

@danielguillan danielguillan commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

showControlsWhenPaused used to control whether VideoPlayer rendered its controls while paused. That behavior was removed in 0.49.0 because hiding/showing the controls changed the player height on play/pause, causing layout shift.

Since the prop has already been non-functional, this PR removes the remaining public API and documentation references instead of reintroducing behavior or keeping a deprecated no-op.

List of notable changes:

  • Removed showControlsWhenPaused from the VideoPlayer TypeScript prop surface
  • Removed showControlsWhenPaused from Storybook controls
  • Removed showControlsWhenPaused from the VideoPlayer docs props table
  • Updated the changeset to document the prop removal

Steps to test:

  1. Run npm run build:lib.
  2. Run npm run test --workspace=packages/react -- --testPathPatterns="VideoPlayer".
  3. Run npm run check --workspace=packages/react.
  4. Run npm run build:next-docs.
  5. Confirm showControlsWhenPaused is no longer listed in the VideoPlayer docs, Storybook controls, or generated TypeScript declarations.

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Not applicable. This removes a non-functional prop from API/docs surfaces only.

Copilot AI review requested due to automatic review settings July 6, 2026 16:00
@danielguillan
danielguillan requested a review from a team as a code owner July 6, 2026 16:00
@changeset-bot

changeset-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 137bc23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@primer/react-brand Patch
@primer/brand-docs Patch
@primer/brand-css Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🟢 No design token changes found

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🟢 No unit test coverage changes found

All components and hooks with tests maintain the same coverage as the main branch.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the VideoPlayer component to properly support (and type) the showControlsWhenPaused prop, allowing consumers to hide the control bar while paused, and adds Storybook + test coverage around the new behavior.

Changes:

  • Added showControlsWhenPaused?: boolean to VideoPlayer props with a default of true.
  • Updated controls bar visibility logic to hide the controls bar when paused if showControlsWhenPaused={false}.
  • Added Storybook control and new unit tests covering paused vs playing behavior.
Show a summary per file
File Description
packages/react/src/VideoPlayer/VideoPlayer.tsx Adds the showControlsWhenPaused prop and updates controls-bar rendering logic.
packages/react/src/VideoPlayer/VideoPlayer.test.tsx Adds tests asserting controls are hidden while paused (but shown while playing) when the prop is false.
packages/react/src/VideoPlayer/VideoPlayer.stories.tsx Adds a Storybook boolean control + default arg for showControlsWhenPaused.
.changeset/smart-wolves-pause.md Adds a changeset entry describing the update.

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Low

Comment thread packages/react/src/VideoPlayer/VideoPlayer.tsx
Comment thread .changeset/smart-wolves-pause.md Outdated
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🟢 Bundle size report

CheckMainBranchChange
UMD — full bundle (JS)101.33 kB101.33 kB🟢 No change
UMD — full bundle (CSS)66.82 kB66.86 kB⬆️ +38 B (+0.1%)
ESM — full bundle (JS + CSS)1.51 MB1.51 MB✅ -69 B (-0.0%)
ESM — tree-shaken simple (Button)69.70 kB69.59 kB✅ -111 B (-0.2%)
ESM — tree-shaken complex (ActionMenu)78.48 kB78.37 kB✅ -111 B (-0.1%)

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@danielguillan
danielguillan marked this pull request as draft July 6, 2026 17:20
@danielguillan danielguillan changed the title Fix VideoPlayer showControlsWhenPaused prop Deprecate VideoPlayer showControlsWhenPaused prop Jul 7, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielguillan
danielguillan marked this pull request as ready for review July 7, 2026 16:17
@danielguillan

Copy link
Copy Markdown
Collaborator Author

@rezrah I think we could simply remove the prop completely instead of marking it as deprecated, as it hasn't been working since v0.49. Any thoughts?

@rezrah

rezrah commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

@rezrah I think we could simply remove the prop completely instead of marking it as deprecated, as it hasn't been working since v0.49. Any thoughts?

Agree, we should just remove it unless we have a 100% working solution for it (no CLS impact).

I'm a bit unclear on whether we need to remove or restore original functionality though, can you confirm? If it's genuinely not needed, let's remove it.

@danielguillan danielguillan changed the title Deprecate VideoPlayer showControlsWhenPaused prop Remove non-functional VideoPlayer showControlsWhenPaused prop Jul 9, 2026
@danielguillan

Copy link
Copy Markdown
Collaborator Author

It is safe to remove since it has been non-functional. There is a temporary solution to work around VideoPlayer limitations for that use case, and https://github.com/github/brand-experience/issues/383 is planned. 👍

@danielguillan
danielguillan merged commit 21c954b into main Jul 16, 2026
16 checks passed
@danielguillan
danielguillan deleted the danielguillan/fix-video-player branch July 16, 2026 13:25
@primer primer Bot mentioned this pull request Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants